Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0126] Standardize special properties #126

Closed
wants to merge 1 commit into from

Conversation

schuelermine
Copy link

@schuelermine schuelermine commented May 26, 2022

@schuelermine schuelermine force-pushed the rfc-0126 branch 3 times, most recently from 85500c8 to e660a39 Compare May 26, 2022 15:54
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-standardize-magic-properties/19344/1

# Detailed design
[design]: #detailed-design

Properties treated specially by the Nix tools and Nix language when dealing with arbitrary are renamed to start with a double underscore. Instances where this is not necessary are `__functor` and `__toString`. `recurseForDerivations` is renamed `__recurseForDerivations` and `type` is renamed `__type`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Properties treated specially by the Nix tools and Nix language when dealing with arbitrary are renamed to start with a double underscore. Instances where this is not necessary are `__functor` and `__toString`. `recurseForDerivations` is renamed `__recurseForDerivations` and `type` is renamed `__type`.
Properties treated specially by the Nix tools and Nix language when dealing with are arbitrarily renamed to start with a double underscore. Instances where this is not necessary are `__functor` and `__toString`. `recurseForDerivations` is renamed `__recurseForDerivations` and `type` is renamed `__type`.

# Motivation
[motivation]: #motivation

Nix treats many attributes specially with unique behaviours. Some of these are prefixed with double underscores to disambiguate them from everything else. But some aren’t: `recurseForDerivations` isn’t, `type`, when set to `"derivation"`, changes visual output.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hard time understanding the exact problem and its severity.

Could you please make an explicit list of the magic attributes you are aware of, explain what they are for, why they are magic, and link to relevant documentation and source code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that there’s a bunch of specially treated attribute names that mean attrsets aren’t neutral data structures.
Maybe magic is a bad word.
As I say further down, I don’t know a full list, since these seem to be things that aren’t considered together in the documentation.

type is magic because if type = "derivation", the data is displayed completely differently in the Nix repl.
By renaming them to start with __, I hope to make them more obvious, more segregated from normal attributes, and easier to deal with.

A problem this could fix could arise when a user who does not know how a process works inputs data into it, which is unknowingly processed to create an attribute that unexpectedly alters the result’s behaviour.
None exist ATM AFAIK, but they could.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attrsets by themselves are (almost?) neutral, I believe, but some functions consuming them treat some attributes specially.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the exception of __functor. And precisely these, in my opinion, should be dunder(__)-prefixed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is magic because if type = "derivation", the data is displayed completely differently in the Nix repl.
By renaming them to start with __, I hope to make them more obvious, more segregated from normal attributes, and easier to deal with.

it's even more magic than that. changing type is a flag day event, and for that reason alone probably not worth it. also note that recurseForDerivations isn't (afawk( honored by the new nix commands at all either.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a good point. Looking at the code though it seems not so hard to change. Don’t the new commands try to build non-derivation store path results anyways? But yeah, this would be a big change, probably not worth it. Also, AFAIK nix-command honors recurseForDerivations in legacyPackages only.

# Drawbacks
[drawbacks]: #drawbacks

- This change is backwards-incompatible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's such a good idea to make it actually breaking: IIRC one of the benefits Nix advertises is that you can instantiate old Nix expressions and still get the same result (but that also means that the behavior of the expression shouldn't change).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but then this reduces to just guardMagic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this doesn’t stop old expressions from building, since __toString and __functor are the only ones here that aren’t user-facing, and they’re not renamed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When grepping for recurseForDerivations in nixpkgs, there's a bunch of places where it's explicitly used, so what do you mean with "user-facing"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Their special effect is outside the expression language. AFAIK recurseForDerivations tells nix commands to search for derivations deeper down, but it’s only being set in Nixpkgs, not used by it. Renaming it without renaming it in nixpkgs or using an old nix expression will not affect directly building any particular derivation, but only auxiliary things like searching. It’s unlikely that many nix expression directly check recurseForDerivations.
The other ones, on the other hand, if changed, would actually stop some Nix expressions from evaluating. __functor concerns the in-expression behaviour of these values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, adding synonyms that start with __ for all the magic attributes and deciding that Nixpkgs will use the underscore-based things from now on is more than guardMagic.

Evaluation-affecting magic seems to be indeed underscored; nix command behaviour is nominally still experimental so some changes are possible with proper warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluation-affecting magic seems to be indeed underscored; nix command behaviour is nominally still experimental so some changes are possible with proper warning.

But doesn't that affect nix-* commands usually as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. In my opinion that’s not as much a breaking change as a change in how some derivations build.

@schuelermine schuelermine changed the title [RFC 0126] Standardize magic properties [RFC 0126] Standardize special properties May 27, 2022
@schuelermine
Copy link
Author

I have updated this PR to say "special" instead of "magic", I think that’s better.

@bew
Copy link

bew commented May 27, 2022

I have updated this PR to say "special" instead of "magic", I think that’s better.

Please also change the 'Rendered' link in first post (:

@edolstra edolstra added status: open for nominations Open for shepherding team nominations and removed status: new labels Jun 15, 2022
@edolstra
Copy link
Member

This RFC is now open for shepherd nominations!

# Detailed design
[design]: #detailed-design

Properties treated specially by the Nix tools and Nix language are renamed to start with a double underscore. Instances where this is not necessary are `__functor` and `__toString`. `recurseForDerivations` is renamed `__recurseForDerivations` and `type` is renamed `__type`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recurseForDerivations is everywhere, not just in Nixpkgs.
Do we have any evidence of this causing a problem in the real world?
I suggest adapting the definition of builtins.guardSpecial instead.

Same goes for type.
Note that we also have _type, which is the type tag used by module system related constructors. Again, changing this is more trouble than it's worth. Just add _type to guardSpecial's deny list.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t that have to be a completely different RFC?

Note that I’m open to closing this for good, it’s a much bigger change than I had anticipated when I wrote it up.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to expand on the "any evidence of this causing a problem" bit: changing attribute names would be incredibly hard in the very dynamic language nix is. we can find all instances of e.g. drvPath in code, but which of these are used to construct/modify derivations and which aren't? meta is to some extent a special name, changing that would touch almost the entire nixpkgs tree (and introduce mismatches between derivations and modules). changing old names feels like asking for a disaster, and silently dropping assignments can also easily turn into a footgun. 😕

which is not to say that guarding against accidental use of special names is bad! such a guard should (usually) fail very loudly though to catch mistakes early, behaving much like assert ! elem name [ «specials» ] && match "__.*" name == null; name. prefixing new special names also sounds very reasonable.

@edolstra
Copy link
Member

@fricklerhandwerk @roberth @Ma27 @pennae Are any of you interested in shepherding this RFC?

@fricklerhandwerk
Copy link
Contributor

@edolstra No. I‘m not qualified, both in terms of nixpkgs and RFC expertise, and also don’t want to dilute my focus on documentation work.

@schuelermine
Copy link
Author

A note: I am no longer necessarily in favor of this RFC, even though I authored it. . The comments have convinced me this is too big an undertaking. Maybe it should be closed? I’m sorry for cluttering up the RFC process in that case.

@schuelermine
Copy link
Author

schuelermine commented Jun 29, 2022

GitHub appears to have changed the behaviour (on Firefox, at least) of Ctrl+Enter to closing instead of commenting… Sorry about that.

@pennae
Copy link

pennae commented Jun 30, 2022

@edolstra no, don't feel up to the task (and don't think the scope outlined in the RFC is good at all, if anything it should be cut down to "new attributes with special interpretation by nix should have a __ prefix", which is common practice already)

@roberth
Copy link
Member

roberth commented Jun 30, 2022

@edolstra The original idea behind this RFC doesn't work out in cost/benefit. I wouldn't mind doing a bit more guidance, but I don't think we need the full RFC process to carry this to a satisfactory conclusion.

@schuelermine Functions like you describe could be added to the Nixpkgs lib without controversy, when they don't impose your original breaking changes.
In my opinion, for this outcome, it's better not to go through RFCs, because such an RFC would only be useful for describing the status quo, and it may go out of date when the implementation of the functions is improved. (For documentation, we have the manual)

@lheckemann
Copy link
Member

@schuelermine if you feel that there's anything useful this RFC could be rewritten to do instead, feel free; otherwise you're also welcome to withdraw it (just close it) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: open for nominations Open for shepherding team nominations
Projects
None yet
Development

Successfully merging this pull request may close these issues.